Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove max gas #188

Closed
wants to merge 2 commits into from
Closed

Remove max gas #188

wants to merge 2 commits into from
+6 −9

Conversation

ethanfrey
Copy link
Member

Closes #170

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added relevant godoc comments.

  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md

  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@ethanfrey ethanfrey mentioned this pull request Jul 14, 2020
Closed
webmaster128
webmaster128 approved these changes Jul 14, 2020
View reviewed changes
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic diff looks good

alpe
alpe approved these changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

x/wasm/internal/keeper/keeper_test.go
@@ -299,13 +300,17 @@ func TestInstantiate(t *testing.T) {
require.NoError(t, err)

gasBefore := ctx.GasMeter().GasConsumed()
fmt.Printf("limit: %d\n", ctx.GasMeter().Limit())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: t.Logf should be preferred to fmt.Printf for test debug output

alpe
alpe suggested changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@alpe alpe left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 A test is failing in CI. On my box I got:

   keeper_test.go:314:
        	Error Trace:	keeper_test.go:314
        	Error:      	Not equal:
        	            	expected: 0x11542
        	            	actual  : 0x28f5c28efc6754a
        	Test:       	TestInstantiate

@ethanfrey
Copy link
Member Author

Yeah, sorry, I added the commit message, but forgot to add it to the description

This works for everything but one place, and I don't know why it fails there (thus the Printf). I will try to look later, unless someone else wants to take a look

@ethanfrey ethanfrey mentioned this pull request Jul 15, 2020
Merged
9 tasks
@ethanfrey
Copy link
Member Author

Closing in favor of #193

@ethanfrey ethanfrey closed this Jul 15, 2020
@ethanfrey ethanfrey deleted the remove-max-gas branch July 15, 2020 14:23
zemyblue pushed a commit to Finschia/wasmd that referenced this pull request Jan 2, 2023
@tac0turtle @jackzampolin
Merge PR CosmWasm#188: Add linting to CI
967fc43 
* Add some linting to gaia

- closes CosmWasm#187

Signed-off-by: Marko Baricevic <[email protected]>

* fail tests

* tests pass

* address PR comments

* some more strictness
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alpe alpe alpe requested changes

@webmaster128 webmaster128 webmaster128 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove keeper.MaxGas or write a comment why it is needed
3 participants
@ethanfrey @alpe @webmaster128